sequenceDiagram
    participant Node
    participant CacheManager
    participant Cache
    participant Store
    participant DARetriever

    Node->>CacheManager: RestoreFromStore()
    activate CacheManager
    CacheManager->>Store: Read snapshotKey
    Store-->>CacheManager: snapshot (or empty)
    alt snapshot present
        CacheManager->>Cache: Restore in-flight height→DA entries (placeholders)
    end
    CacheManager->>Store: Read finalized-tip metadata (HeightToDAHeight)
    Store-->>CacheManager: max DA height metadata
    CacheManager->>Cache: Initialize maxDAHeight
    deactivate CacheManager

    DARetriever->>CacheManager: setNodeHeightToDAHeight(height, hash)
    activate CacheManager
    alt hash present in cache
        CacheManager->>Cache: lookup by hash → daHeight
    else hash missing (post-restart)
        CacheManager->>Cache: HeightPlaceholderKey lookup → placeholder daHeight
    end
    CacheManager->>Store: Persist HeightToDAHeight metadata
    deactivate CacheManager

    Node->>CacheManager: SaveToStore()
    activate CacheManager
    CacheManager->>Cache: Encode current snapshot
    Cache-->>CacheManager: snapshot bytes
    CacheManager->>Store: Write snapshotKey
    deactivate CacheManager

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Claude finished @julienrbrt's task in 2m 15s —— View job

Code Review: refactor(store,cache)!: optimize cache restore as O(1)

• Gather context and understand the PR
• Review cache snapshot implementation (generic_cache.go)
• Review cache manager changes (manager.go)
• Review submitter changes (submitter.go)
• Review store changes and test coverage
• Review test quality and coverage
• Compile findings and post review

Summary

This is a well-motivated refactor. The core idea—replacing a per-entry store scan with a single consolidated snapshot key—is correct and clean. The placeholder-based restore mechanism elegantly handles the restart window before the DA retriever can re-fire real content hashes. The test suite is comprehensive. Below are findings from most to least critical.

Issues

1. decodeSnapshot silently masks data corruption — generic_cache.go:245-256

When the buffer is non-empty but not a multiple of snapshotEntrySize (e.g. due to partial write or bitflip), decodeSnapshot returns nil, causing RestoreFromStore to behave as if the snapshot were empty. The node silently loses all in-flight DA state rather than surfacing the corruption.

// Current — bad: treats corrupt data as "nothing to restore"
func decodeSnapshot(buf []byte) []snapshotEntry {
    if len(buf) == 0 || len(buf)%snapshotEntrySize != 0 {
        return nil
    }
    ...
}

The fix (already suggested by CodeRabbit) is to return an error for the malformed case:

func decodeSnapshot(buf []byte) ([]snapshotEntry, error) {
    if len(buf) == 0 {
        return nil, nil
    }
    if len(buf)%snapshotEntrySize != 0 {
        return nil, fmt.Errorf("malformed snapshot: len=%d, expected multiple of %d", len(buf), snapshotEntrySize)
    }
    ...
    return entries, nil
}

RestoreFromStore should then propagate the error rather than continue with no state. Fix this →

2. persistSnapshot called with context.Background() — generic_cache.go:154,161,204

func (c *Cache[T]) setDAIncluded(hash string, daHeight uint64, blockHeight uint64) {
    ...
    _ = c.persistSnapshot(context.Background()) // bypasses caller's context
}

The caller context is ignored, so if the store becomes unavailable the write hangs until context.Background() is satisfied (i.e., never). The _ = silently discards the error too. This is intentional for best-effort persistence, but the "silently discard" pattern means store failures during mutation are invisible. Consider at least logging a warning:

if err := c.persistSnapshot(context.Background()); err != nil {
    // log.Warn or handle; at minimum, don't fully silently discard
}

3. Test helper has unused parameters — manager_test.go:117

writeHeightDAMeta := func(height, daH uint64, _, _ string) {
    ...
}
writeHeightDAMeta(1, 100, "", "")
writeHeightDAMeta(2, 101, "", "")

The two string parameters are never used. This compiles but is misleading and will fail golangci-lint's unparam or revive checks. Should be:

writeHeightDAMeta := func(height, daH uint64) { ... }
writeHeightDAMeta(1, 100)
writeHeightDAMeta(2, 101)

Fix this →

Observations & Nits

Concurrency in setMaxDAHeight — generic_cache.go:171-180

func (c *Cache[T]) setMaxDAHeight(daHeight uint64) {
    for range 1_000 {
        ...
        if c.maxDAHeight.CompareAndSwap(current, daHeight) {
            return
        }
    }
}

The 1_000-iteration CAS retry is unusual. Under extreme pathological contention (1000 concurrent writers all racing on this), the update would silently be dropped. In practice this is fine—DA heights advance slowly—but the magic number deserves a comment explaining why it was chosen and what happens if it's exceeded.

initDAHeightFromStore reads store metadata but ignores errors — manager.go:393-409

func (m *implementation) initDAHeightFromStore(ctx context.Context) {
    daIncludedBytes, err := m.store.GetMetadata(ctx, store.DAIncludedHeightKey)
    if err != nil || len(daIncludedBytes) != 8 {
        return
    }
    ...
}

This is intentional best-effort seeding (no error return), but a store read failure on startup is silently swallowed, leaving DaHeight() at zero and potentially causing the syncer to re-scan from genesis. This may be an acceptable trade-off, but the comment block would benefit from explicitly documenting this: "if DAIncludedHeightKey is absent or unreadable, DaHeight() starts at 0 and the syncer will catch up."

ClearFromStore rebuilds all 4 caches — manager.go:379-383

m.headerCache = NewCache[types.SignedHeader](m.store, HeaderDAIncludedPrefix)
m.dataCache = NewCache[types.Data](m.store, DataDAIncludedPrefix)
m.txCache = NewCache[struct{}](nil,)
m.pendingEventsCache = NewCache[common.DAHeightEvent](nil,)

This is correct but worth verifying there are no concurrent readers holding stale references to the old headerCache/dataCache at the time ClearFromStore is called. Since NewManager invokes this during initialization (before goroutines start), it's safe—but a comment noting that ClearFromStore is only safe to call before the cache is shared would prevent future misuse.

IsHeightDAIncluded / setNodeHeightToDAHeight already use height-based lookup as primary — submitter.go:435,499

Both methods now call GetHeaderDAIncludedByHeight / GetDataDAIncludedByHeight directly (not as a fallback after a hash-based miss). This correctly addresses alpe's comment from the review thread.

What's Good

• Architecture: The single-snapshot-key approach is significantly cleaner than the previous per-entry key model. The O(1) restore claim holds for the snapshot read itself; the O(k) decode (k = in-flight window) is bounded and tiny.
• Placeholder eviction: The orphan-prevention logic in setDAIncluded (evicting the old placeholder when the real hash arrives) is correct and well-tested.
• Error handling: Previous versions silently dropped SaveToStore/ClearFromStore errors; this PR propagates them correctly.
• RestoreFromStore error discrimination: Correctly treats ds.ErrNotFound as a no-op and propagates other errors.
• Test coverage: TestCache_RestartIdempotent, TestCache_NoPlaceholderLeakAfterRefire, TestSubmitter_IsHeightDAIncluded_AfterRestart, and TestSubmitter_setNodeHeightToDAHeight_AfterRestart are particularly valuable regression tests.
• Store cleanup: Removing GetMetadataByPrefix is a good simplification—the prefix scan was replaced by more targeted lookups.
• Timer fix in executor: max(..., 0) instead of negative clamp is correct.

Required Before Merge

Codecov Report

❌ Patch coverage is 85.84071% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.12%. Comparing base (b1e3010) to head (007ac1c).
⚠️ Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3134      +/-   ##
==========================================
+ Coverage   61.00%   61.12%   +0.12%     
==========================================
  Files         113      113              
  Lines       11700    11676      -24     
==========================================
  Hits         7137     7137              
+ Misses       3753     3736      -17     
+ Partials      810      803       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.